Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shadowsocks/shadowsocks-android#2562 #278

Closed
wants to merge 1 commit into from
Closed

Conversation

madeye
Copy link
Contributor

@madeye madeye commented Jul 16, 2020

@madeye madeye changed the title Fix shadowsocks-android/#2562 Fix shadowsocks/shadowsocks-android#2562 Jul 16, 2020
@Mygod
Copy link
Contributor

Mygod commented Jul 16, 2020

The latest openssl 0.10.30 pakcage requires a minimal SDK target 23

Why is this?

Also there is a better fix imo for this.

@zonyitoo
Copy link
Collaborator

The latest openssl 0.10.30 pakcage requires a minimal SDK target 23

That's bad.. :(

@Mygod
Copy link
Contributor

Mygod commented Jul 16, 2020

shadowsocks/shadowsocks-android@0d75659

We should try to find a fix for 0.10.30 as well. Perhaps the new prefab thing? https://android-developers.googleblog.com/2020/02/native-dependencies-in-android-studio-40.html

@madeye
Copy link
Contributor Author

madeye commented Jul 17, 2020

Maybe we just increase the minSDK to 23...

@Mygod
Copy link
Contributor

Mygod commented Jul 17, 2020

Where did you find the information regarding minSDK?

@RebelliousWhiz
Copy link

I don't know what's impacting the code, but to my perspective, people still use SDK 23 devices don't care about GFW.... My grandma has a MI 6, and it's now on Android 10...

I guess it's completely fine to set minSDK to 23.

@Mygod
Copy link
Contributor

Mygod commented Jul 17, 2020

Reported to upstream: sfackler/rust-openssl#1315

If we do not get a satisfactory fix we can drop support for Android 5. (openssl updates > legacy Android support)

@madeye madeye force-pushed the android-fix-2562 branch from b0569e5 to dc13f3b Compare July 19, 2020 04:25
@zonyitoo
Copy link
Collaborator

So, is it ok for me to merge this MR? @Mygod

@Mygod
Copy link
Contributor

Mygod commented Jul 20, 2020

I don't think we need this merged for now.

@madeye
Copy link
Contributor Author

madeye commented Jul 20, 2020

I think the missing part is how to pass VPN option to plugins, which is not defined before.

Maybe we should fix this from plugin's side instead.

@Mygod
Copy link
Contributor

Mygod commented Jul 20, 2020

So let's settle this once and for all: what should we use? -V or V in plugin options.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 20, 2020

Since SS_PLUGIN_OPTIONS is defined as "options for plugin", the VPN flag should be passed in options.

Some programs will exit immediately if it meets unrecognized arguments, which is why I don't think it is a good idea to implicitly add -V for all plugins.

BTW, v2ray-plugin supports vpn in option: https://github.com/shadowsocks/v2ray-plugin/blob/master/main.go#L332-L334 .

I propose vpn in plugin options instead of V in plugin options or -V in program arguments, which has more precise meaning than V. V means a lot of things, version, verbose, ...

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 20, 2020

Or maybe we can define a new environment variable, such as SS_PLUGIN_CTRL, which could be used for passing standardized controlling options, like vpn_mode=1&verbose=3&fast_open=1

@madeye
Copy link
Contributor Author

madeye commented Jul 21, 2020

Let's put V into the SS_PLUGIN_OPTIONS. I'll release a new v2ray-plugin to ensure the VPU flags is parsed correctly.

@madeye
Copy link
Contributor Author

madeye commented Jul 21, 2020

@Mygod
Copy link
Contributor

Mygod commented Jul 23, 2020

Should we get rid of this hack then?

if opt.contains(";V") {

@madeye
Copy link
Contributor Author

madeye commented Jul 23, 2020

Yep, let's remove that hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5.1.2 doesn't work with v2ray-plugin (with default plugin configuration)
4 participants